Skip to content

Support file_fixture in Factory definitions #427

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

seanpdoyle
Copy link
Contributor

Related to factory_bot#1282

rails/rails#45606 has been merged and is likely to be released as part of Rails 7.1.

With that addition, the path toward resolving factory_bot#1282 becomes more clear. If factories can pass along Pathname instances to attachment attributes, Active Support will handle the rest.

Instances of ActiveSupport::TestCase provide a file_fixture helper to construct a Pathname instance based on the path defined by ActiveSupport::TestCase.file_fixture_path (relative to the Rails root directory).

Copy link

@dorianmariecom dorianmariecom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would test an actual file upload if possible. Otherwise looks good to me

@mike-burns
Copy link
Contributor

This is our first time extending FactoryBot::SyntaxRunner. Do you think this should be something people opt in to? Will this break anything?

I can't think of how this could break existing tests but maybe I'm just not being creative enough.

@seanpdoyle seanpdoyle force-pushed the file-fixture-path-support branch from 8a739aa to 5a1a15e Compare October 25, 2024 19:14
@seanpdoyle seanpdoyle marked this pull request as ready for review October 25, 2024 19:14
@seanpdoyle seanpdoyle force-pushed the file-fixture-path-support branch 6 times, most recently from 0cd66df to 33c1348 Compare November 1, 2024 03:40
@seanpdoyle seanpdoyle force-pushed the file-fixture-path-support branch from 33c1348 to 61baf5f Compare January 10, 2025 20:44
@seanpdoyle seanpdoyle force-pushed the file-fixture-path-support branch 7 times, most recently from 611f31f to f878964 Compare January 10, 2025 23:06
Related to [factory_bot#1282][]

[rails/rails#45606][] has been merged and is likely to be released as
part of Rails 7.1.

With that addition, the path toward resolving [factory_bot#1282][]
becomes more clear. If factories can pass along [Pathname][] instances
to attachment attributes, Active Support will handle the rest.

Instances of `ActiveSupport::TestCase` provide a [file_fixture][] helper
to construct a `Pathname` instance based on the path defined by
`ActiveSupport::TestCase.file_fixture_path` (relative to the Rails root
directory).

[factory_bot#1282]: thoughtbot/factory_bot#1282 (comment)
[rails/rails#45606]: rails/rails#45606
[Pathname]: https://docs.ruby-lang.org/en/master/Pathname.html
[file_fixture]: https://api.rubyonrails.org/classes/ActiveSupport/Testing/FileFixtures.html#method-i-file_fixture
@seanpdoyle seanpdoyle force-pushed the file-fixture-path-support branch from f878964 to 43e18b9 Compare January 10, 2025 23:12
@seanpdoyle seanpdoyle merged commit e8f750c into main Jan 17, 2025
18 checks passed
@seanpdoyle seanpdoyle deleted the file-fixture-path-support branch January 17, 2025 15:36
@runephilosof-abtion
Copy link

This should be added to the NEWS as well (under a unreleased header)

seanpdoyle added a commit that referenced this pull request Jan 21, 2025
Follow-up to [#427][]

Add an entry to the `NEWS.md` file to document support for
[file_fixture][] added in [#427][].

[#427]: #427
[file_fixture]: https://api.rubyonrails.org/classes/ActiveSupport/Testing/FileFixtures.html#method-i-file_fixture
@seanpdoyle seanpdoyle mentioned this pull request Jan 21, 2025
seanpdoyle added a commit that referenced this pull request Jan 27, 2025
Follow-up to [#427][]

Add an entry to the `NEWS.md` file to document support for
[file_fixture][] added in [#427][].

[#427]: #427
[file_fixture]: https://api.rubyonrails.org/classes/ActiveSupport/Testing/FileFixtures.html#method-i-file_fixture
seanpdoyle added a commit that referenced this pull request Jan 31, 2025
Follow-up to [#427][]

Add an entry to the `NEWS.md` file to document support for
[file_fixture][] added in [#427][].

[#427]: #427
[file_fixture]: https://api.rubyonrails.org/classes/ActiveSupport/Testing/FileFixtures.html#method-i-file_fixture
neilvcarvalho pushed a commit that referenced this pull request Jan 31, 2025
Follow-up to [#427][]

Add an entry to the `NEWS.md` file to document support for
[file_fixture][] added in [#427][].

[#427]: #427
[file_fixture]: https://api.rubyonrails.org/classes/ActiveSupport/Testing/FileFixtures.html#method-i-file_fixture
@tsvallender tsvallender mentioned this pull request Jun 7, 2025
@pedantic-git
Copy link

pedantic-git commented Jun 19, 2025

Hi @seanpdoyle - here's an anecdote for you in case it's useful elsewhere. Having the option to opt out of this turned out to be necessary (and worked).

My tests already included fixture support, which I had mixed into FactoryBot in a different way here.

After updating to 6.5.0, my Rspec tests continued to pass but my integration tests (written in Spinach) all started to fail with the same error:

file_fixture delegated to self.class.file_fixture_support, but self.class.file_fixture_support is nil
        /home/runner/work/name.pn/name.pn/vendor/bundle/ruby/3.4.0/gems/factory_bot_rails-6.5.0/lib/factory_bot_rails/file_fixture_support.rb:6:in 'FactoryBot::SyntaxRunner#file_fixture'

I think this is because the Railtie assumes the testing system is Rspec, and if it's something other than Rspec, it leaves self.class.file_fixture_support as nil. It might be worth having some documentation about how to add this support into non-Rspec testing environments, since I'd prefer to use your upstream support rather than my home-rolled one, but I couldn't quite figure out how it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants